Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try to make etc/ci_test.sh work #1033

Merged
merged 6 commits into from
Sep 10, 2024
Merged

Try to make etc/ci_test.sh work #1033

merged 6 commits into from
Sep 10, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Sep 4, 2024 •
edited
Loading

etc/ci_test.sh gets called as part of the testsuite of GAP.jl.

Already for a long time, gcov prints some errors in CI, see e.g. https://github.com/oscar-system/GAP.jl/actions/runs/10700854945/job/29665504736#step:6:468. On newer gcov version (my system uses gcc/gcov 14.2.1, while the ubuntu CI runners use 11.2.0), instead of just printing the error, this returns a non-zero exit code and thus aborts the script.
This PR contains three small changes in the hopes to fix this:

  1. The gcov contained a wrong path. This is changed for JuliaInterface. Since JuliaExperimental doesn't have any c files anymore, gcov cannot give any coverage for them. So this line gets disabled.
  2. The build flags of JuliaInterface now contain the needed flag to create coverage files during compilation. This would of course be nice to have only in CI cases and not always, but I don't know how to do that.
  3. I added a force re-compilation of JuliaInterface before running its tests. On CI, this shouldn't change anything, but locally, I had an issue that changes to the Makefile (see previous point) didn't get picked up automatically. This forces it instead.

Alternatively, we could just remove the two lines regarding gcov from etc/ci_test.sh completely.

cc @ThomasBreuer

@codecov Codecov
Copy link

codecov bot commented Sep 4, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.45%. Comparing base (e82078d) to head (faba40b).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/julia-config.jl 0.00% 6 Missing ⚠️
src/setup.jl 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1033      +/-   ##
==========================================
- Coverage   76.49%   74.45%   -2.04%     
==========================================
  Files          51       55       +4     
  Lines        4191     4534     +343     
==========================================
+ Hits         3206     3376     +170     
- Misses        985     1158     +173
Files with missing lines Coverage Δ
src/setup.jl 85.47% <0.00%> (+3.41%) ⬆️
src/julia-config.jl 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

@lgoettgens lgoettgens marked this pull request as ready for review September 5, 2024 08:19
fingolfin
fingolfin reviewed Sep 5, 2024
View reviewed changes
Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!!

etc/ci_test.sh Outdated Show resolved Hide resolved
etc/ci_test.sh Outdated Show resolved Hide resolved
pkg/JuliaInterface/Makefile.in Outdated Show resolved Hide resolved
@lgoettgens

This comment was marked as outdated.

Sign in to view
lgoettgens
lgoettgens commented Sep 5, 2024
View reviewed changes
etc/ci_test.sh Outdated
@@ -9,17 +9,18 @@ mkdir -p coverage
#
cd pkg/JuliaInterface
pwd
# Force recompilation of JuliaInterface with coverage instrumentation
CFLAGS=--coverage FORCE_JULIAINTERFACE_COMPILATION=true ${GAP} --nointeract
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this CFLAGS variable does not get forwarded to the process that compiles JuliaInterface.

GAP.jl/src/setup.jl

Line 221 in a8674da

withenv("CFLAGS" => JULIA_CPPFLAGS,

Instead, this just uses the output of GAP.Setup.cflags() as the CFLAGS environment var. So this CFLAGS var here does not do anything.

@lgoettgens
Copy link
Member Author

@ThomasBreuer noticed that gcov reports 0% coverage for all files, so something was off. As mentioned in #1033 (comment), the CFLAGS env var did not get forwarded. Instead, I now introduced JULIAINTERFACE_WITH_COVERAGE that sets the coverage flag at the appropriate location in setup.jl, and additionally adds the needed linker flag.
Locally running ]test now reports non-zero coverage in the gcov output

fingolfin
fingolfin reviewed Sep 5, 2024
View reviewed changes
src/setup.jl Outdated Show resolved Hide resolved
@fingolfin fingolfin closed this Sep 10, 2024
@fingolfin fingolfin reopened this Sep 10, 2024
@fingolfin fingolfin merged commit 73fbfa3 into oscar-system:master Sep 10, 2024
39 of 42 checks passed
@fingolfin
Copy link
Member

Thank you @lgoettgens !

@lgoettgens lgoettgens deleted the lg/gcov branch September 10, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fingolfin fingolfin fingolfin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lgoettgens @fingolfin